Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

store/tikv: make snapshot thread safe (#17593) #18145

Merged
merged 11 commits into from
Jun 30, 2020
Merged

store/tikv: make snapshot thread safe (#17593) #18145

merged 11 commits into from
Jun 30, 2020

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Cherry pick #17593 to release-4.0

Problem Summary:

A correct fix need to cherry pick a series of commits:

#16056
#17555
#17593
#17678
#17911

That maybe too much for 4.0

What is changed and how it works?

make snapshot thread safe

Check List

Tests

  • Unit test

Release note

  • No release note

@tiancaiamao
Copy link
Contributor Author

PTAL @coocood @lysu

@github-actions github-actions bot added the sig/execution SIG execution label Jun 19, 2020
@ti-srebot
Copy link
Contributor

@coocood,Thanks for you review.

@coocood
Copy link
Member

coocood commented Jun 28, 2020

@tiancaiamao
Just move code from Next to Open doesn't completely fix the bug.
The PointGet executor maybe built many times during execution. for example, in IndexJoin.

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jun 29, 2020 •
edited
Loading

@tiancaiamao
Just move code from Next to Open doesn't completely fix the bug.
The PointGet executor maybe built many times during execution. for example, in IndexJoin.

Move code from Next to Open so the snapshot is not captured everytime and reduce conflict (the transaction is modified and snapshot may be nil)

This code is not safe, as txn maybe set to nil after Valid() and before GetSnapshot.

if txn.Valid() {
     snapshot = txn.GetSnapshot()
}

I override GetSnapshot to provide the snapshot.
Not in a atomic way, but it should work , as a pointer is in a machine word size (assumption on hardware), either nil or a kv.Snapshot is OK.

@tiancaiamao
Copy link
Contributor Author

Link issue #16817

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@tiancaiamao
Copy link
Contributor Author

github is unavailable

@tiancaiamao tiancaiamao reopened this Jun 29, 2020
@coocood
Copy link
Member

coocood commented Jun 29, 2020

LGTM

@ti-srebot
Copy link
Contributor

@coocood, Thanks for your review, however we are sorry that your vote won't be count. You already give a LGTM to this PR

@coocood
Copy link
Member

coocood commented Jun 29, 2020

@bobotu PTAL

bobotu
bobotu previously approved these changes Jun 29, 2020
View reviewed changes
Copy link
Contributor

@bobotu bobotu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in a atomic way, but it should work , as a pointer is in a machine word size (assumption on hardware), either nil or a kv.Snapshot is OK.

This may have a problem with ARMv7?

In particular, the model was originally non-multicopy-atomic: writes could become visible to some other threads before becoming visible to all

Anyway, it isn't a blocker for this PR.

LGTM

@ti-srebot
Copy link
Contributor

@bobotu, Thanks for your review, however we are sorry that your vote won't be count. You are not a reviewer or committer or co-leader or leader for the related sigs:execution(slack).

coocood
coocood reviewed Jun 30, 2020
View reviewed changes
session/txn.go Outdated Show resolved Hide resolved
coocood
coocood reviewed Jun 30, 2020
View reviewed changes
session/txn.go Outdated Show resolved Hide resolved
@coocood
Copy link
Member

coocood commented Jun 30, 2020

LGTM

@ti-srebot
Copy link
Contributor

@coocood, Thanks for your review, however we are sorry that your vote won't be count. You already give a LGTM to this PR

@ti-srebot
Copy link
Contributor

@SunRunAway,Thanks for you review.

@jebter
Copy link

jebter commented Jun 30, 2020

/merge

2 similar comments
@jebter
Copy link

jebter commented Jun 30, 2020

/merge

@zhouqiang-cl
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 30, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@tiancaiamao merge failed.

@jebter
Copy link

jebter commented Jun 30, 2020

/run-common-test

@jebter
Copy link

jebter commented Jun 30, 2020

/run-check_dev

@jebter jebter merged commit 64f64fc into pingcap:release-4.0 Jun 30, 2020
@ichn-hu ichn-hu mentioned this pull request Sep 2, 2020
Closed
@tiancaiamao tiancaiamao deleted the cherry-17593 branch September 7, 2020 07:59
@tiancaiamao tiancaiamao mentioned this pull request Sep 7, 2020
Merged
@tiancaiamao tiancaiamao mentioned this pull request Jan 20, 2021
Merged
@AilinKid AilinKid mentioned this pull request Jan 20, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@coocood coocood coocood left review comments

@SunRunAway SunRunAway SunRunAway approved these changes

@bobotu bobotu bobotu left review comments

Assignees
No one assigned
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. type/4.0-cherry-pick
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tiancaiamao @ti-srebot @coocood @jebter @zhouqiang-cl @SunRunAway @bobotu